Skip to content

Conversation

hanickadot
Copy link
Contributor

Fixes #80285

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. labels Feb 1, 2024
@llvmbot
Copy link
Member

llvmbot commented Feb 1, 2024

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Hana Dusíková (hanickadot)

Changes

Fixes #80285


Full diff: https://github.com/llvm/llvm-project/pull/80292.diff

2 Files Affected:

  • (modified) clang/lib/CodeGen/CoverageMappingGen.cpp (+14-2)
  • (modified) clang/test/CoverageMapping/if.cpp (+29)
diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 8b5e6c4ad8272..6640fe6f41fd1 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -1808,12 +1808,24 @@ struct CounterCoverageMappingBuilder
     }
   }
 
+private:
+  static bool evaluateConstantCondition(const Expr *Condition) {
+    if (const auto *Expr = dyn_cast<ConstantExpr>(Condition))
+      return Expr->getResultAsAPSInt().getExtValue();
+
+    if (const auto *Expr = dyn_cast<ExprWithCleanups>(Condition))
+      return evaluateConstantCondition(Expr->getSubExpr()); // recursion
+
+    assert(false && "Unexpected node in 'if constexpr' condition");
+    return false;
+  }
+
+public:
   void coverIfConstexpr(const IfStmt *S) {
     assert(S->isConstexpr());
 
     // evaluate constant condition...
-    const auto *E = cast<ConstantExpr>(S->getCond());
-    const bool isTrue = E->getResultAsAPSInt().getExtValue();
+    const bool isTrue = evaluateConstantCondition(S->getCond());
 
     extendRegion(S);
 
diff --git a/clang/test/CoverageMapping/if.cpp b/clang/test/CoverageMapping/if.cpp
index 3045ffe43948c..4de1467aa7ee3 100644
--- a/clang/test/CoverageMapping/if.cpp
+++ b/clang/test/CoverageMapping/if.cpp
@@ -234,6 +234,35 @@ constexpr int check_macro_consteval_if_skipped(int i) {   // CHECK-NEXT: [[@LINE
   return i;
 }
 
+struct false_value {
+  constexpr operator bool() {
+    return false;
+  }
+};
+
+template <typename> struct dependable_false_value {
+  constexpr operator bool() {
+    return false;
+  }
+};
+
+// GH-80285
+void should_not_crash() {
+    if constexpr (false_value{}) { };
+}
+
+template <typename> void should_not_crash_dependable() {
+    if constexpr (dependable_false_value<int>{}) { };
+}
+
+void should_not_crash_with_template_instance() {
+  should_not_crash_dependable<int>();
+}
+
+void should_not_crash_with_requires_expr() {
+   if constexpr (requires {42;}) { };
+}
+
 int instantiate_consteval(int i) {
   i *= check_consteval_with_else_discarded_then(i);
   i *= check_notconsteval_with_else_discarded_else(i);

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM, but please add a release note to clang/docs/ReleaseNotes.rst so users know about the fix.

Thank you for the fix!

@hanickadot
Copy link
Contributor Author

Changes LGTM, but please add a release note to clang/docs/ReleaseNotes.rst so users know about the fix.

Thank you for the fix!

Even if it's a fix for recent PR I did few days ago?

Also, how do I get this into release branch (the previous change is there)?

@cor3ntin
Copy link
Contributor

cor3ntin commented Feb 1, 2024

Changes LGTM, but please add a release note to clang/docs/ReleaseNotes.rst so users know about the fix.

Thank you for the fix!

FYI this is a regression in 18 affecting users so we should backport

@hanickadot hanickadot removed openmp:libomp OpenMP host runtime openmp:libomptarget OpenMP offload runtime labels Feb 1, 2024
@AaronBallman
Copy link
Collaborator

Changes LGTM, but please add a release note to clang/docs/ReleaseNotes.rst so users know about the fix.
Thank you for the fix!

Even if it's a fix for recent PR I did few days ago?

Oh, I had missed that, sorry! Then no, a release note isn't needed.

@AaronBallman
Copy link
Collaborator

Changes LGTM, but please add a release note to clang/docs/ReleaseNotes.rst so users know about the fix.
Thank you for the fix!

FYI this is a regression in 18 affecting users so we should backport

Btw, instructions on how to do that can be found here: https://www.llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches

@hanickadot hanickadot merged commit bfc6eaa into llvm:main Feb 1, 2024
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 1, 2024
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 2, 2024
agozillon pushed a commit to agozillon/llvm-project that referenced this pull request Feb 5, 2024
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
@pointhex pointhex mentioned this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen IR generation bugs: mangling, exceptions, etc. clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[coverage] crash with ExprWithCleanups in if constexpr
6 participants